fix(yabeda): Normalize plural Yabeda units to Sentry's singular form#2953
Conversation
Yabeda plugins declare units as plural Ruby symbols (:seconds, :milliseconds, :bytes) while Sentry's canonical unit format is singular (second, millisecond, byte). Without normalization, distribution metrics with units are stored under the wrong unit string, causing dashboard queries to return empty results. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
This PR has been automatically closed. The referenced issue does not show a discussion between you and a maintainer. To avoid wasted effort on both sides, please discuss your proposed approach in the issue first and wait for a maintainer to respond before opening a PR. Please review our contributing guidelines for more details. |
dingsdax
added
Bug
Ruby
Metrics
and removed
violating-contribution-guidelines
missing-maintainer-discussion
labels
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| # in the Sentry product. See https://develop.sentry.dev/sdk/foundations/state-management/scopes/attributes/#units | ||
| def unit_for(metric) | ||
| metric.unit&.to_s | ||
| metric.unit&.to_s&.chomp("s") |
There was a problem hiding this comment.
Bug: The use of chomp("s") to handle pluralization will incorrectly truncate any custom unit that naturally ends in "s", like :process.
Severity: MEDIUM
Suggested Fix
Instead of unconditionally calling chomp("s"), use a more robust method for singularization, such as a dedicated inflection library. Alternatively, if only handling standard units, use a targeted replacement based on a known list of plural forms rather than a blanket string manipulation.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: sentry-yabeda/lib/sentry/yabeda/adapter.rb#L72
Potential issue: The `unit_for` method unconditionally calls `.chomp("s")` on the string
representation of a metric's unit. This is problematic for custom units whose names
legitimately end in "s", such as `:process` or `:status`. In these cases, the unit will
be incorrectly truncated to `"proces"` or `"statu"` before being sent to Sentry. This
results in a mangled, unrecognized unit in Sentry without raising any error or warning,
leading to silent data corruption for those metrics. Standard Sentry units are
unaffected as they do not end in 's' in their singular form.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
that's right but ok in our case, as we do have a fixed set of units for now imho
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8694506. Configure here.
| # in the Sentry product. See https://develop.sentry.dev/sdk/foundations/state-management/scopes/attributes/#units | ||
| def unit_for(metric) | ||
| metric.unit&.to_s | ||
| metric.unit&.to_s&.chomp("s") |
There was a problem hiding this comment.
Naive chomp("s") corrupts abbreviated unit names
Low Severity
Using chomp("s") to normalize units will mangle any unit whose singular/canonical form ends in "s". Abbreviated units like :ms, :ops, or :fps would become "m", "op", or "fp". The test "leaves already-singular units unchanged" uses :second (ending in "d"), so it doesn't actually exercise this risky scenario and gives false confidence in the approach.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 8694506. Configure here.
2 participants
Description
Normalize plural Yabeda unit symbols (e.g.
:seconds,:milliseconds,:bytes) to Sentry's canonical singular form (e.g."second","millisecond","byte") inSentry::Yabeda::Adapter#unit_for.Yabeda registers units as plural Ruby symbols, but the Sentry units spec expects singular strings. Previously
unit_forjust called.to_s, sending unrecognized plural units. This change uses.chomp("s")to strip the trailing"s".Changes
sentry-yabeda/lib/sentry/yabeda/adapter.rb: Replace.to_swith.to_s.chomp("s")and remove the TODO commentsentry-yabeda/spec/sentry/yabeda/adapter_spec.rb: Update existing unit expectations to singular form; add dedicated "unit normalization" test group covering plural→singular, milliseconds, nil, and already-singular casessentry-yabeda/spec/sentry/yabeda/integration_spec.rb: Update integration spec expectation to"millisecond"